Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "Add InstrumentationLibrary to Sampler.ShouldSample (#1850)" #1942

Merged
merged 2 commits into from
Sep 22, 2021
Merged

Revert "Add InstrumentationLibrary to Sampler.ShouldSample (#1850)" #1942

merged 2 commits into from
Sep 22, 2021

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Sep 20, 2021 •
edited
Loading

This reverts commit f936f2e.

During today Maintainers meeting @jmacd expressed his concern about #1658 and the direction that it moves SDK: runtime decisions in Samplers as opposed to configuration-time decisions of configuring SDK and/or Tracers with the correct Samplers. His opinion was that we should use the latter: all decisions based on Resources and InstrumentationLibraries can be done before providing Tracers to the users. Thus those Tracers can already use Samplers chosen/narrowed down based on that information. Thus they don't need to accept it in ShouldSample method.

Several attendees, myself including, agreed that this is a reasonable idea. It was thus decided to revert introducing InstrumentationLibrary to ShouldSample method and get back to the drawing board to choose between two approaches:
either pass both Resources and InstrumentationLibrary to ShouldSample in one spec release OR don't pass neither of them and allow SDKs to configure specific Sampler for a specific Tracer.

@iNikem iNikem requested review from a team September 20, 2021 16:46
Oberon00
Oberon00 previously requested changes Sep 20, 2021
View reviewed changes
Copy link
Member

@Oberon00 Oberon00 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed during today Maintainers meeting.

Please provide a more detailed justification for those that did not attend (feel free to dismiss my review after that)

@yurishkuro
Copy link
Member

yurishkuro commented Sep 20, 2021 •
edited
Loading

Please provide a more detailed justification for those that did not attend (feel free to dismiss my review after that)

+1. Do we need to improve our community guidelines to make it clear that "was discussed in a meeting" is never a justification for any change?

@Oberon00 Oberon00 added area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Sep 20, 2021
@iNikem
Copy link
Contributor Author

iNikem commented Sep 20, 2021

Sorry, updated PR's description

@iNikem iNikem dismissed Oberon00’s stale review September 20, 2021 17:31

Concern addressed

@Oberon00
Copy link
Member

Please ensure a follow-up issue for this new configuration design is created before merging.

@jsuereth
Copy link
Contributor

@Oberon00 I'm not sure what you mean here. Can you clarify? I'd like to understand what you'd like to see before moving this PR forward.

@Oberon00
Copy link
Member

Oberon00 commented Sep 21, 2021 •
edited
Loading

Quoting the description:

get back to the drawing board to choose between two approaches:
either pass both Resources and InstrumentationLibrary to ShouldSample in one spec release OR don't pass neither of them and allow SDKs to configure specific Sampler for a specific Tracer.

There should be an issue for that. Probably it will be best to use the existing #1588, copying & pasting the gist to that issuewould make sense. And of course, adding a "Closes #1588" to the description to close the PR for adding resources, which then wouldn't make sense at all.

@iNikem
Copy link
Contributor Author

iNikem commented Sep 22, 2021

Added #1588 (comment)

@Oberon00 Oberon00 mentioned this pull request Sep 22, 2021
Closed
@carlosalberto
Copy link
Contributor

@Oberon00 Wanted to double confirm you are fine with Nikita's comment, and we can merge this one?

@Oberon00
Copy link
Member

I need to correct my previous comment: I meant to suggest "Closes #1658", #1588 is obviously not resolved.

But yes, I'm OK with merging this PR, although with a somewhat uneasy feeling, as I fear this will keep #1588 in the backlog for quite some time.

@jsuereth jsuereth merged commit 6d44e2e into open-telemetry:main Sep 22, 2021
@iNikem iNikem deleted the revert-instrumentation-library branch September 22, 2021 15:42
@srikanthccv srikanthccv mentioned this pull request Sep 24, 2021
Closed
@cijothomas cijothomas mentioned this pull request Feb 21, 2024
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@iNikem @bogdandrutu
Revert "Add InstrumentationLibrary to Sampler.ShouldSample (open-tele…
3b003ee 
…metry#1850)" (open-telemetry#1942)

This reverts commit f936f2e.

Co-authored-by: Bogdan Drutu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmacd jmacd jmacd approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@Oberon00 Oberon00 Oberon00 left review comments

Assignees

@jsuereth jsuereth

Labels
area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@iNikem @yurishkuro @Oberon00 @jsuereth @carlosalberto @bogdandrutu @jmacd